Channel logging improvements#4242
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4242 +/- ##
==========================================
- Coverage 89.33% 89.32% -0.01%
==========================================
Files 180 180
Lines 138680 139331 +651
Branches 138680 139331 +651
==========================================
+ Hits 123888 124460 +572
- Misses 12172 12238 +66
- Partials 2620 2633 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @jkczyz! This PR has been waiting for your review. |
valentinewallace
left a comment
valentinewallace
left a comment
There was a problem hiding this comment.
I like having more logs in these pipelines 👌
lightning/src/ln/channelmanager.rs
Outdated
| for action in actions.into_iter() { | ||
| match action { | ||
| MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim } => { | ||
| let logger = WithContext::from(&self.logger, None, None, Some(payment_hash)); |
There was a problem hiding this comment.
You could include some extra info here:
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index 63ecdd1fb..e799a0bda 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -9451,7 +9451,7 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
for action in actions.into_iter() {
match action {
MonitorUpdateCompletionAction::PaymentClaimed { payment_hash, pending_mpp_claim } => {
- let logger = WithContext::from(&self.logger, None, None, Some(payment_hash));
+ let logger = WithContext::from(&self.logger, pending_mpp_claim.as_ref().map(|c| c.0), pending_mpp_claim.as_ref().map(|c| c.1), Some(payment_hash));
log_trace!(logger, "Handling PaymentClaimed monitor update completion action");
if let Some((counterparty_node_id, chan_id, claim_ptr)) = pending_mpp_claim {
valentine@Valentines-MacBook-Pro rust-lightning %There was a problem hiding this comment.
Good addition. Rewrote it a bit.
Additional trace logs to help with debugging.
joostjager
force-pushed
the
extra-logs
branch
from
0647af3 to
f312c24
Compare
|
Tacked on rustfmt for updated method. |
lightning/src/ln/channelmanager.rs
Outdated
| for (cp, cid) in pending_claim_state | ||
| .channels_with_preimage | ||
| .iter() | ||
| { |
There was a problem hiding this comment.
IMO rustfmt mangles this code a bit at the moment.
Suggestion:
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index ea6409d0e..cd0adaaef 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -9465,58 +9465,41 @@ This indicates a bug inside LDK. Please report this error at https://github.com/
WithContext::from(&self.logger, peer_id, chan_id, Some(payment_hash));
log_trace!(logger, "Handling PaymentClaimed monitor update completion action");
- if let Some((counterparty_node_id, chan_id, claim_ptr)) = pending_mpp_claim {
+ if let Some((cp_node_id, chan_id, claim_ptr)) = pending_mpp_claim {
let per_peer_state = self.per_peer_state.read().unwrap();
- per_peer_state.get(&counterparty_node_id).map(|peer_state_mutex| {
+ per_peer_state.get(&cp_node_id).map(|peer_state_mutex| {
let mut peer_state = peer_state_mutex.lock().unwrap();
let blockers_entry =
peer_state.actions_blocking_raa_monitor_updates.entry(chan_id);
if let btree_map::Entry::Occupied(mut blockers) = blockers_entry {
blockers.get_mut().retain(|blocker| {
- if let &RAAMonitorUpdateBlockingAction::ClaimedMPPPayment {
- pending_claim,
- } = &blocker
- {
- if *pending_claim == claim_ptr {
- let mut pending_claim_state_lock =
- pending_claim.0.lock().unwrap();
- let pending_claim_state =
- &mut *pending_claim_state_lock;
- pending_claim_state.channels_without_preimage.retain(
- |(cp, cid)| {
- let this_claim = *cp == counterparty_node_id
- && *cid == chan_id;
- if this_claim {
- pending_claim_state
- .channels_with_preimage
- .push((*cp, *cid));
- false
- } else {
- true
- }
- },
- );
- if pending_claim_state
- .channels_without_preimage
- .is_empty()
- {
- for (cp, cid) in pending_claim_state
- .channels_with_preimage
- .iter()
- {
- let freed_chan = (*cp, *cid, blocker.clone());
- freed_channels.push(freed_chan);
- }
- }
- !pending_claim_state
- .channels_without_preimage
- .is_empty()
+ let pending_claim = match &blocker {
+ RAAMonitorUpdateBlockingAction::ClaimedMPPPayment {
+ pending_claim,
+ } => pending_claim,
+ _ => return true,
+ };
+ if *pending_claim != claim_ptr {
+ return true;
+ }
+ let mut claim_state_lock = pending_claim.0.lock().unwrap();
+ let claim_state = &mut *claim_state_lock;
+ claim_state.channels_without_preimage.retain(|(cp, cid)| {
+ let this_claim = *cp == cp_node_id && *cid == chan_id;
+ if this_claim {
+ claim_state.channels_with_preimage.push((*cp, *cid));
+ false
} else {
true
}
- } else {
- true
+ });
+ if claim_state.channels_without_preimage.is_empty() {
+ for (cp, cid) in claim_state.channels_with_preimage.iter() {
+ let freed_chan = (*cp, *cid, blocker.clone());
+ freed_channels.push(freed_chan);
+ }
}
+ !claim_state.channels_without_preimage.is_empty()
});
if blockers.get().is_empty() {
blockers.remove();There was a problem hiding this comment.
Applied patch, even though I think we should not do such hardly related refactorings. We've had a bug introduced before because of making rustfmt look nice.
|
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
valentinewallace
merged commit fd85279
into
lightningdevkit:main
3 participants
Additional trace logs that helped with debugging #4218 and might be generally useful.